Skip to content

Conversation

@asbeane
Copy link

@asbeane asbeane commented Mar 21, 2025

Attempts to resolve #319

checks for the ordinal (in parameterized tests), and this case maps each test case to the method name and wraps each of these in their own test suite so the tests show up with each parameterized test as test case, and the method has a test suite.

Basically the hierarchy (for parameterized tests)

Test Class (also a test suiteA)
Test case/method
[1] params=...
[2] params=....

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR. This seems okay, but I've a couple of questions:

  1. How does Gradle and/or Maven report this in their log files? We should be compatible with that, since those are widely used tools, and the XML files are sucked into CI runners to display results.
  2. Can we please have a test for this? Without one, we're likely to regress this output as we continue to work in this area.

@shs96c
Copy link
Collaborator

shs96c commented Jul 25, 2025

We recently landed #351. Could you please LMK if this addressed your issue, or whether this PR still needs to be landed.

@asbeane
Copy link
Author

asbeane commented Jul 25, 2025

We recently landed #351. Could you please LMK if this addressed your issue, or whether this PR still needs to be landed.

Hey @shs96c, apologies for being so slow. @mattnworb brought this back to my attention.
So I tested this today, and it looks like the previous change (#351) nicely added support for nested tests, but (from my own testing, and from what I can tell) did not add support for parameterized tests (which this PR does).

To answer your previous question - the intent of this PR is to reach compatibility with what maven does (at least to the best of my knowledge), specifically for Parameterized Tests.

I'll work on adding a test this per your suggestion - thank you for the reminder and follow-up @shs96c!

@christianladam
Copy link
Contributor

christianladam commented Jul 25, 2025

We recently landed #351. Could you please LMK if this addressed your issue, or whether this PR still needs to be landed.

Hey @shs96c, apologies for being so slow. @mattnworb brought this back to my attention. So I tested this today, and it looks like the previous change (#351) nicely added support for nested tests, but (from my own testing, and from what I can tell) did not add support for parameterized tests (which this PR does).

To answer your previous question - the intent of this PR is to reach compatibility with what maven does (at least to the best of my knowledge), specifically for Parameterized Tests.

I'll work on adding a test this per your suggestion - thank you for the reminder and follow-up @shs96c!

Yes #351 was done specifically to handle nesting and kept the previous behavior for parameterized tests alone on purpose for the XML because we wanted to match the behavior of gradle/maven in terms of grouping by class and I wanted the keep the changes focused. Technically one can specify a name value to the annotation to fix this but it's less than ideal behavior when one doesn't do that. The scenarios are:

  1. no names in the annotations
    @ParameterizedTest

  2. name set for the ParameterizedTest annotation that includes the template name
    @ParameterizedTest(name = "aTest({0})")

  3. name set for the ParameterizedTest annotation that doesn't include the template name (or includes the template name with a bunch of other stuff also)
    @ParameterizedTest(name = "just some customized string {0}")

Then have something like:
@valuesource(strings = {"one", "two", "three", "four"})
void aTest(String value) {
...
}

What's the desired behavior here?

@christianladam
Copy link
Contributor

christianladam commented Jul 25, 2025

For reference here are the XML formats for the same parameterized test class in bazel, gradle, and maven (when no name is specified in the annotation):

BAZEL

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="ParameterisedTest" timestamp="2025-07-25T23:19:11.359936Z" hostname="Macbook-Pro.local" tests="3" time="0.03" failures="0" errors="0" disabled="0" skipped="0" package="">
    <properties/>
    <testcase name="[2] goGreek=beta" classname="com.github.bazel_contrib.contrib_rules_jvm.junit5.ParameterisedTest" time="0">
      <system-out><![CDATA[beta
]]></system-out>
    </testcase>
    <testcase name="[1] goGreek=alpha" classname="com.github.bazel_contrib.contrib_rules_jvm.junit5.ParameterisedTest" time="0.01">
      <system-out><![CDATA[alpha
]]></system-out>
    </testcase>
    <testcase name="[3] goGreek=gamma" classname="com.github.bazel_contrib.contrib_rules_jvm.junit5.ParameterisedTest" time="0">
      <system-out><![CDATA[gamma
]]></system-out>
    </testcase>
  </testsuite>
</testsuites>

GRADLE: https://github.com/bazel-contrib/rules_jvm/blob/main/comparative-tests/build/test-results/test/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml

MAVEN (surefire): https://github.com/bazel-contrib/rules_jvm/blob/main/comparative-tests/target/surefire-reports/TEST-com.github.bazel_contrib.contrib_rules_jvm.comparative.ParameterisedTest.xml

The main differences include the bazel XML having an outer <testsuites> (to match the default JUnit4 test runner's xml), maven and bazel sharing the same system-out data while gradle has a list outside of the testcases as well as having a system-err line, and the naming being:

bazel [index] parameterName=value
gradle [index] value
maven methodName(type)[index]

@christianladam
Copy link
Contributor

christianladam commented Jul 26, 2025

After looking into this more, repeated and parameterized tests are using the default naming scheme defined in the junit5 code. I'm not sure the code should override that by default.

There's multiple ways to override the default naming behavior including doing things like creating your own NameFormatters and registering yours to override the default ones. Specifically for ParameterizedTests It's probably just easier to go ahead and add the following to your .bazelrc to test:

test --jvmopt="-Djunit.jupiter.params.displayname.default=\"[{index}] {displayName}=({arguments})\""

Which would make the output go from:
[1] goGreek=alpha
to:
[1] bootstrap(String)=(alpha)

See https://docs.junit.org/current/user-guide/#writing-tests-display-names for more details.

@asbeane
Copy link
Author

asbeane commented Jul 28, 2025

Cool, thank you @christianladam! Taking a look.

@asbeane
Copy link
Author

asbeane commented Jul 29, 2025

@christianladam - using jvmopt worked for me. Happy to close this for now. Thank you for the help!

@asbeane asbeane closed this Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BazelJUnitOutputListener doesn't log methodName for @ParameterizedTest

3 participants